opt: generate lookup joins on partitioned indexes #57690
Conversation
Release note: None
|
This change is |
mgartner
force-pushed
the
partitioned-lookup-join
branch
from
0ab91a6
to
7868c86
Compare
| @@ -1110,27 +1110,41 @@ limit | |||
| │ ├── fd: (10)-->(15) | |||
| │ ├── project | |||
| │ │ ├── columns: quantity:14!null accountname:10!null | |||
| │ │ ├── inner-join (hash) | |||
| │ │ ├── inner-join (lookup inventorydetails) | |||
There was a problem hiding this comment.
This new query plan is better I think. Previously it was scanning inventorydetails (a large table) and only constraining by accountname (low cardinality). The new plan is a lookup join which is a "pseudo-scan" constraining accountname, dealerid, and cardid. What do you think @andy-kimball?
Previously, the optimizer could create lookup join keys from filters that constrain a column to a single constant value. This was done by wrapping the join input in a Project that projected the constant value, and using this new column as a key column. This commit generalizes this behavior so that lookup join keys can also be created from filters that constrain a column to multiple, non-ranging constant values. The constant values are cross-joined with the input, and the joined column is used as a key column. If a column is constrained to a single constant value, the cross join normalizes to a Project identical to the Projects constructed prior to this commit. Release note (performance improvement): The query optimizer can use filters that constrained columns to multiple constant values to generate lookup joins. For example, a join filter `x.a = y.a AND y.b IN (1, 2)` can be used to generate a lookup join on table `y` assuming that it has an index on `(a, b)` or `(b, a)`.
mgartner
force-pushed
the
partitioned-lookup-join
branch
from
7868c86
to
c5a5ab2
Compare
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @mgartner, @RaduBerinde, and @rytaft)
pkg/sql/opt/xform/testdata/rules/join, line 3011 at r5 (raw file):
│ │ │ ├── (10,) │ │ │ ├── (20,) │ │ │ └── (30,)
This is interesting. What do we currently do with geo-partitioned regular indices when the input rows do not supply the partition column values and the values are injected using a predicate like the b IN (10, 20, 30) here. Does this mean we were unable to use the partitioned index currently?
Will this cross-join with a constant table also be how we inject these constants into the input row for feeding into the inverted join?
Do we know enough (in the optimizer) after doing this cross-join to ship the inputs prefixed by 10, 20, 30 to the relevant regions (when this is a partitioning field), so that the lookup can be local?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/xform/testdata/rules/join, line 3011 at r5 (raw file):
Does this mean we were unable to use the partitioned index currently?
Yes. Prior to this PR, we use partitioned indexes for other expressions like scans, but not for lookup joins.
Will this cross-join with a constant table also be how we inject these constants into the input row for feeding into the inverted join?
Yes, that's the plan. I had started work on inverted joins on partitioned inverted indexes, and realized that an inverted join is similar to a lookup join on a non-inverted index. Once I realized that lookup joins did not utilize partitioned indexes, I decided to start with that as a "warm-up" to the inverted join work.
Do we know enough (in the optimizer) after doing this cross-join to ship the inputs prefixed by 10, 20, 30 to the relevant regions (when this is a partitioning field), so that the lookup can be local?
I'm not 100% sure, but I think this information is available at the distsql level but not in the optimizer. @RaduBerinde or @rytaft may be able to answer this better.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/xform/testdata/rules/join, line 3295 at r5 (raw file):
---- opt expect=GenerateLookupJoinsWithFilter
[nit] maybe explain that the inlining rule converts the single-left row cross join with project
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 3 of 5 files at r2, 12 of 12 files at r4, 11 of 11 files at r5.
Reviewable status:
pkg/sql/opt/xform/testdata/rules/join, line 3011 at r5 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
Does this mean we were unable to use the partitioned index currently?
Yes. Prior to this PR, we use partitioned indexes for other expressions like scans, but not for lookup joins.
Will this cross-join with a constant table also be how we inject these constants into the input row for feeding into the inverted join?
Yes, that's the plan. I had started work on inverted joins on partitioned inverted indexes, and realized that an inverted join is similar to a lookup join on a non-inverted index. Once I realized that lookup joins did not utilize partitioned indexes, I decided to start with that as a "warm-up" to the inverted join work.
Do we know enough (in the optimizer) after doing this cross-join to ship the inputs prefixed by 10, 20, 30 to the relevant regions (when this is a partitioning field), so that the lookup can be local?
I'm not 100% sure, but I think this information is available at the
distsqllevel but not in the optimizer. @RaduBerinde or @rytaft may be able to answer this better.
@mgartner is right that this would happen at the DistSQL planner level, not at the optimizer level. But I'm pretty sure that what you're describing is not how lookup joins are planned, even by the DistSQL planner. Although @RaduBerinde or someone from SQL Execution would know better here.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/xform/testdata/rules/join, line 3011 at r5 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
@mgartner is right that this would happen at the DistSQL planner level, not at the optimizer level. But I'm pretty sure that what you're describing is not how lookup joins are planned, even by the DistSQL planner. Although @RaduBerinde or someone from SQL Execution would know better here.
I think the lookup joins are currently distributed based on the locations the input happens to be generated at. If the input was already geo-partitioned, that seems fine. If we've generated the partitioning columns using the cross-join, like this example, there may be cases where "moving" the input to the relevant regions may be cheaper. For example, if the input is small, but the output of the join is large and is subsequently aggregated.
I am not suggesting any changes for this PR -- just curious about how query execution placement in a multi-region setting will behave.
|
pkg/sql/opt/xform/testdata/rules/join, line 3011 at r5 (raw file): Previously, sumeerbhola wrote…
Yea, that's a good point. We'll have to see if this is a common use case, in which case it may be worth doing in the DistSQL planner. It seems non-trivial, though. Also, this might be something we can plan in the optimizer some day (we are eventually hoping to make some of the distribution decisions inside the optimizer). |
mgartner
force-pushed
the
partitioned-lookup-join
branch
from
c5a5ab2
to
7110d5e
Compare
Previously, only explicit filters were used to generated lookup join key columns. Now lookup join keys can be generated from CHECK constraints and computed column expressions. With this commit and the previous commit, lookup joins on partitioned indexes are explored by the optimizer. Release note (performance improvement): The query optimizer now explores plans with lookup joins on partitioned indexes, resulting in more efficient query plans in some cases.
mgartner
force-pushed
the
partitioned-lookup-join
branch
from
7110d5e
to
1a3fd72
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/xform/testdata/rules/join, line 3295 at r5 (raw file):
Previously, RaduBerinde wrote…
[nit] maybe explain that the inlining rule converts the single-left row cross join with project
This happens in quite a few tests in this file, so instead of commenting on each one, I added some commentary to the GenerateLookupJoins function.
|
pkg/sql/opt/xform/join_funcs.go, line 306 at r6 (raw file):
Reminder from our discussion just now that this should probably not copy the join private |
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/xform/join_funcs.go, line 306 at r6 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
Reminder from our discussion just now that this should probably not copy the join private
Thanks! @rytaft @RaduBerinde PTAL at the new commit.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/xform/join_funcs.go, line 306 at r6 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
Thanks! @rytaft @RaduBerinde PTAL at the new commit.
Also please confirm that my assumption that "reordering a cross-join is pointless". Or is there a benefit in a cross-join to having fewer rows in the left child than the right?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/xform/join_funcs.go, line 306 at r6 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
Also please confirm that my assumption that "reordering a cross-join is pointless". Or is there a benefit in a cross-join to having fewer rows in the left child than the right?
Well, we store the right side in memory completely (with fallback to disk if necessary). So if we're cross-joining 2 rows with 1M rows, we really want to store the small side.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/xform/join_funcs.go, line 306 at r6 (raw file):
Previously, RaduBerinde wrote…
Well, we store the right side in memory completely (with fallback to disk if necessary). So if we're cross-joining 2 rows with 1M rows, we really want to store the small side.
If a lookup join is chosen as the optimal plan, its unlikely that either side of this child cross-join is very large. A lookup is performed for each output row of the cross-join, which would be costly if the cross-join produced many rows.
It seems sane to me to leave the constants on the right and not attempt reordering. The constants are a single column, so even in cases where there are more constants than input rows its plausible that the constants would have a smaller memory footprint.
But if you think its safer to permit reordering of this cross-join then I'm happy to. Also happy to try to benchmark the cost of this reordering vs no reordering if that helps us decide which direction to take.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/xform/join_funcs.go, line 306 at r6 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
If a lookup join is chosen as the optimal plan, its unlikely that either side of this child cross-join is very large. A lookup is performed for each output row of the cross-join, which would be costly if the cross-join produced many rows.
It seems sane to me to leave the constants on the right and not attempt reordering. The constants are a single column, so even in cases where there are more constants than input rows its plausible that the constants would have a smaller memory footprint.
But if you think its safer to permit reordering of this cross-join then I'm happy to. Also happy to try to benchmark the cost of this reordering vs no reordering if that helps us decide which direction to take.
Having the constants on the right make sense, but why disable reordering? We should let other rules run as usual unless we have a strong reason to disable them.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r6, 3 of 3 files at r7.
Reviewable status:
pkg/sql/opt/xform/join_funcs.go, line 306 at r6 (raw file):
Previously, RaduBerinde wrote…
Having the constants on the right make sense, but why disable reordering? We should let other rules run as usual unless we have a strong reason to disable them.
Yea, I'd also err on the side of just allowing reordering. I don't see a benefit in disabling it here.
Might also be worth adding a test that using an INNER LOOKUP JOIN hint works (since using the INNER INVERTED JOIN hint was how we identified the issue with copying the join private in the other case).
mgartner
force-pushed
the
partitioned-lookup-join
branch
2 times, most recently
from
b507d14
to
8015b08
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/xform/join_funcs.go, line 306 at r6 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
Yea, I'd also err on the side of just allowing reordering. I don't see a benefit in disabling it here.
Might also be worth adding a test that using an
INNER LOOKUP JOINhint works (since using the INNER INVERTED JOIN hint was how we identified the issue with copying the join private in the other case).
SGTM. I've added a test with INNER LOOKUP JOIN and explained why the join flags aren't propagated in a comment. And updated the commit message and PR description.
This commit fixes a bug that prevented a `LOOKUP` join hint from producing a plan with a lookup join. Previously, the hint was propagated to the synthesized cross join created as input to the lookup join. This artificially inflated the cost of the cross join, making the lookup join too costly to be selected as the optimal plan. Release note: None
mgartner
force-pushed
the
partitioned-lookup-join
branch
from
8015b08
to
4f82ebc
Compare
There was a problem hiding this comment.
Reviewed 4 of 4 files at r8.
Reviewable status:
|
TFTRs! bors r+ |
|
Build succeeded: |
In cockroachdb#57690 a new code path was introduced from `findConstantFilterCols` from `GenerateLookupJoins`. This new code path made it possible for the filters passed to `findConstantFilterCols` to contain columns that are not part of the given table. This violated the assumption that the filter only contains columns in the given table and caused a panic. This commit fixes the issue by neglecting constant filters for columns not in the given table. Fixes cockroachdb#59738 Release note (bug fix): A bug has been fixed that caused errors when joining two tables when one of the tables had a computed column. This bug was present since version 21.1.0-alpha.2 and not present in any production releases.
In cockroachdb#57690 a new code path was introduced from `findConstantFilterCols` from `GenerateLookupJoins`. This new code path made it possible for the filters passed to `findConstantFilterCols` to contain columns that are not part of the given table. This violated the assumption that the filter only contains columns in the given table and caused a panic. This commit fixes the issue by neglecting constant filters for columns not in the given table. Fixes cockroachdb#59738 Release note (bug fix): A bug has been fixed that caused errors when joining two tables when one of the tables had a computed column. This bug was present since version 21.1.0-alpha.2 and not present in any production releases.
54201: roachtest: unskip cdc/crdb-chaos r=aayushshah15 a=aayushshah15 I ran this test a total of 15 times in parallel and wasn't able to reproduce. Since its been skipped for 2+ releases, it's hard to know what fixed it, but a good guess is #48561. Release note: None Fixes #37716 Informs #36879 Release justification: testing only 57170: util/log: new experimental integration with Fluentd r=itsbilal a=knz Release note (cli change): It is now possible to redirect logging to [Fluentd](https://www.fluentd.org)-compatible network collectors. See the documentation for details. This is an alpha-quality feature. 59741: opt: fix panic in GenerateLookupJoin r=mgartner a=mgartner #### opt: fix panic in GenerateLookupJoin In #57690 a new code path was introduced from `findConstantFilterCols` from `GenerateLookupJoins`. This new code path made it possible for the filters passed to `findConstantFilterCols` to contain columns that are not part of the given table. This violated the assumption that the filter only contains columns in the given table and caused a panic. This commit fixes the issue by neglecting constant filters for columns not in the given table. Fixes #59738 Release note (bug fix): A bug has been fixed that caused errors when joining two tables when one of the tables had a computed column. This bug was present since version 21.1.0-alpha.2 and not present in any production releases. #### opt: move findConstantFilterCols to general_funcs.go Release note: None 59779: flowinfra: deflake a test r=yuzefovich a=yuzefovich Previously, a unit test could fail in rare circumstances when relocating a range to a remote node, and now we will use SucceedsSoon to avoid that. Also unskip the vectorized option. Fixes: #59712 Release note: None Co-authored-by: Aayush Shah <aayush.shah15@gmail.com> Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net> Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com> Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
xform: add rule expectations for GenerateLookupJoins tests
Release note: None
opt: generate lookup join for column constrained to multiple constants
Previously, the optimizer could create lookup join keys from filters
that constrain a column to a single constant value. This was done by
wrapping the join input in a Project that projected the constant value,
and using this new column as a key column.
This commit generalizes this behavior so that lookup join keys can also
be created from filters that constrain a column to multiple, non-ranging
constant values. The constant values are cross-joined with the input,
and the joined column is used as a key column. If a column is
constrained to a single constant value, the cross join normalizes to a
Project identical to the Projects constructed prior to this commit.
Release note (performance improvement): The query optimizer can use
filters that constrained columns to multiple constant values to generate
lookup joins. For example, a join filter
x.a = y.a AND y.b IN (1, 2)can be used to generate a lookup join on table
yassuming that it hasan index on
(a, b)or(b, a).opt: generate lookup joins with CHECK constraints and computed columns
Previously, only explicit filters were used to generated lookup join key
columns. Now lookup join keys can be generated from CHECK constraints
and computed column expressions.
With this commit and the previous commit, lookup joins on partitioned
indexes are explored by the optimizer.
Release note (performance improvement): The query optimizer now explores
plans with lookup joins on partitioned indexes, resulting in more
efficient query plans in some cases.
xform: do not propagate join hints to GenerateLookupJoins cross joins
This commit fixes a bug that prevented a
LOOKUPjoin hint fromproducing a plan with a lookup join. Previously, the hint was propagated
to the synthesized cross join created as input to the lookup join. This
artificially inflated the cost of the cross join, making the lookup join
too costly to be selected as the optimal plan.
Release note: None